Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add --target-app-partition argument to flash command #461

Merged
merged 2 commits into from
Nov 4, 2023
Merged

Add --target-app-partition argument to flash command #461

merged 2 commits into from
Nov 4, 2023

Conversation

jnross
Copy link
Contributor

@jnross jnross commented Aug 21, 2023

No description provided.

@jnross jnross closed this Aug 22, 2023
@jnross jnross deleted the flash-to-target-partition branch August 22, 2023 20:17
@jnross jnross restored the flash-to-target-partition branch August 24, 2023 18:36
@jnross jnross reopened this Aug 24, 2023
@jnross
Copy link
Contributor Author

jnross commented Aug 24, 2023

Whoops, I didn't mean to close this. I landed the change in our fork, but shouldn't have deleted the branch.

@jnross jnross force-pushed the flash-to-target-partition branch 2 times, most recently from d42d67b to 3485730 Compare August 25, 2023 04:29
@jnross
Copy link
Contributor Author

jnross commented Aug 25, 2023

I rebased this on top of #465 to get the CI fixes. That should land before this one.

@SergioGasquez SergioGasquez linked an issue Sep 18, 2023 that may be closed by this pull request
Flash application binary into specified partition #459
Closed
@SergioGasquez SergioGasquez changed the title Add --target-app-partition argument to flash command. (For #459) Add --target-app-partition argument to flash command Sep 18, 2023
@SergioGasquez SergioGasquez mentioned this pull request Sep 20, 2023
Merged
@jnross
Copy link
Contributor Author

jnross commented Sep 20, 2023

@SergioGasquez: It's easy to move the argument changes out as you requested. But there are changes to pub function in cli/mod.rs, flasher/mod.rs, and idf_bootloader.rs. I can't see how to avoid those changes and implement this feature without duplicating a large amount of code. Does that mean this change will need to wait until the SemVer major can be bumped to 3? Or is there an additive non-breaking way to add these changes that I'm not seeing?

@SergioGasquez
Copy link
Member

@SergioGasquez: It's easy to move the argument changes out as you requested. But there are changes to pub function in cli/mod.rs, flasher/mod.rs, and idf_bootloader.rs. I can't see how to avoid those changes and implement this feature without duplicating a large amount of code. Does that mean this change will need to wait until the SemVer major can be bumped to 3? Or is there an additive non-breaking way to add these changes that I'm not seeing?

I don't think it's worth to duplicate such large amount of code, I'll mark it as v3 and we can implement it once we work for such version

@SergioGasquez SergioGasquez added this to the v3 milestone Sep 21, 2023
@SergioGasquez
Copy link
Member

Hi! We've just started working towards version 3.0, do you mind rebasing your changes so we can merge them?

@jnross
Copy link
Contributor Author

jnross commented Nov 2, 2023

Yes, will do.

@jnross
Add --target-app-partition argument to flash command. (For esp-rs#459)
4b9ad4c 
# Conflicts:
#	cargo-espflash/src/main.rs
#	espflash/src/bin/espflash.rs
#	espflash/src/cli/mod.rs
#	espflash/src/flasher/mod.rs
#	espflash/src/image_format/idf_bootloader.rs
#	espflash/src/targets/esp32.rs
#	espflash/src/targets/esp32c2.rs
#	espflash/src/targets/esp32c3.rs
#	espflash/src/targets/esp32c6.rs
#	espflash/src/targets/esp32h2.rs
#	espflash/src/targets/esp32s2.rs
#	espflash/src/targets/esp32s3.rs
#	espflash/src/targets/esp8266.rs
#	espflash/src/targets/mod.rs
SergioGasquez
SergioGasquez approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Do you mind updating the CHANGELOG.md?

@jnross
Copy link
Contributor Author

jnross commented Nov 4, 2023

LGTM! Do you mind updating the CHANGELOG.md?

Done!

@SergioGasquez SergioGasquez merged commit 39fbdac into esp-rs:main Nov 4, 2023
19 checks passed
@simpkins simpkins mentioned this pull request May 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergioGasquez SergioGasquez SergioGasquez approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3
Development

Successfully merging this pull request may close these issues.

Flash application binary into specified partition
2 participants
@jnross @SergioGasquez